hmm, we should fix this to be http.server - this is important I believe!

Marked requestHandler spans as http.server 👍

Hmm, do we still instrument outgoing requests without this? Do we really need to remove this? Most otel instrumentation is on top of this...?

Http instrumentation's functionality is covered by requestHandler spans by the opentelemetry-instrumentation-remix.

In this context, HTTP instrumentation also ends up setting an unparameterised route as the root transaction name as I've seen from the tests.

Does the http span become the parent of the remix request handler spans?

If so, could we just extend the HTTP integration for remix to filter out server spans by default? So we still get instrumentation for outgoing requests?

Otherwise I wonder if we should actually vendor in https://github.com/justindsmith/opentelemetry-instrumentations-js/blob/main/packages/instrumentation-remix/src/instrumentation.ts and change it that way?

Yes, makes sense to me, just extended the Http integration the same way it's done in Next.js. Also marked requestHandler spans as http.server.

Is it necessary to run it like this, does it not work with require on top? Just to clarify!

Yes, we need to run it like this. esbuild does not bundle the instrumentation inside the entry files correctly.

hmmm I wonder if this qualifies as a breaking change 😬 thoughts cc @AbhiPrasad ?

I think this does qualify as a breaking change 😬

I guess we have to opt-in to this mode, we can't break the people who already put the work in to upgrade their sdk to v8.

We can make this the default during onboarding though.

I think that makes sense. So I would say we do:

1. Expose remixIntegration but do not add it by default
2. Expose remixHttpIntegration but do not add it by default
3. Users have to add both of these if they want to opt-in (the latter "overwrites" the default http integration)

Does that sound good? 🤔

Questions that remain:

a. Can we find a way to make 2 obsolete? Can't think of a good way to do it sadly 😬
b. Should we then (eventually) document both paths, or just the "new" path? I guess the "old" way could remain as an alternative installation method...?

Can we try adding another init option like useOtel to switch to this mode, and initialise the integrations without (or also with) exposing them?

This change also doesn't affect Express servers (or I guess any other custom server), a top-level import on the server file works in those cases. So we can still get rid of the Express adapter instrumentation, and keep the core instrumentation as an alternative for built-in server users (which I also guess is not very popular among Remix users looking at our issue reports)

I have updated the e2e tests using Express custom servers. (0e2f73a)

Yes, that sounds good - let's make an option for this and adjust default integrations based on this!

Let's refactor this to something like:

function getRemixDefaultIntegrations() {
  return [...]
}

// further down:

if (options.autoInstrumentRemix) {
  options.defaultIntegrations = getRemixDefaultIntegrations(options);
} else {
  instrumentServer();
}

This way users can opt in to the new behavior, but keep the old behavior working if needed?

This also means that wrapExpressCreateRequestHandler should probably remain as it was before, users need to opt-in to not use that anymore...?

Skipping incoming requests worked well both on legacy and otel implementations. I think I managed to keep the old behaviour, without requiring wrapExpressCreateRequestHandler, creating http.server span manually.
Dropping a sample event here

m: Let's re-write this based on the changes done in #12213, which means extracting this into a instrumentRemix method which is called in setupOnce. You'll have to put the options into module scope so they can be updated later...!

Done 👍